Read and basic correlation for execution trace #57

briancoutinho · 2023-07-15T00:44:12Z

What does this PR do?

Add ability to read and correlate execution trace explained in #58

Before submitting

Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
- N/A
Did you write any new necessary tests?
- N/A
Did you make sure to update the docs?
- N/A Feature as a whole is not yet ready, so we can wait till some of the foundational blocks are done
Did you update the changelog?
- N/A

… in example notebook

codecov-commenter · 2023-07-21T22:39:14Z

Codecov Report

Patch coverage: 94.23% and project coverage change: +0.19% 🎉

Comparison is base (d54e43f) 90.26% compared to head (f05e26f) 90.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
+ Coverage   90.26%   90.45%   +0.19%     
==========================================
  Files          27       29       +2     
  Lines        2044     2148     +104     
==========================================
+ Hits         1845     1943      +98     
- Misses        199      205       +6

Files Changed	Coverage Δ
hta/analyzers/cupti_counter_analysis.py	`93.65% <ø> (ø)`
hta/common/execution_trace.py	`91.30% <91.30%> (ø)`
hta/common/trace.py	`87.37% <100.00%> (+0.08%)`	⬆️
tests/test_execution_trace.py	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CHANGELOG.md

anupambhatnagar · 2023-07-24T23:39:35Z

hta/common/execution_trace.py

+        et = ExecutionGraph(json.load(f))
+    t_end = time.perf_counter()
+
+    t_end = time.perf_counter()


why do we need t_end twice?

ooh my bad, copy pasted extra one

anupambhatnagar · 2023-07-24T23:40:19Z

hta/common/execution_trace.py

+
+def _et_has_overlap(trace_df: pd.DataFrame, et: ExecutionGraph) -> bool:
+    """Use record function IDs (rf_id) to find out if ET and Kineto trace
+    have overlap"""


Please follow the same documentation style as in the function above.

I thought this is an internal function hence didn't let me fix this

hta/common/execution_trace.py

anupambhatnagar · 2023-07-24T23:43:53Z

hta/common/execution_trace.py

+    if "et_node" not in trace_df:
+        logger.error("Please run correlate_execution_trace() first")
+        return
+    if column == "op_schema":


what would be returned if any of the if/elif blocks from line 135-164 are reached? only the function definition is called. are the functions executed?

yes the function is used as a lambda / mapping function below. Initially I had lambdas but there was a lint suggesting not assigning lambda to variable.

https://www.flake8rules.com/rules/E731.html#:~:text=Lambdas%20should%20not%20be%20assigned,will%20display%20the%20function's%20name.

anupambhatnagar · 2023-07-24T23:45:23Z

hta/common/trace.py

@@ -91,6 +91,14 @@ def get_sym_id_map(self) -> Dict[str, int]:
    def get_sym_table(self) -> List[str]:
        return self.sym_table

+    def add_back_symbols(self, trace_df: pd.dataframe, col: str) -> None:


What is a "back" symbol? do you mean append symbol to dataframe, as it may have been removed earlier?

yea adding back the string symbols, should i call it expand_symbols() or something?

anupambhatnagar · 2023-07-24T23:46:52Z

requirements.txt

@@ -3,3 +3,4 @@ jupyterlab>=3.5.1
 numpy>=1.23.5
 pandas>=1.5.2
 plotly>=5.11.0
+pydot>=1.3.0


Is this needed? where is it being used?

Let me check if this is required or not, it is being imported in execution trace file so I added it

Is it being added to execution_trace.py or are you referring to some other file?

execution_trace.py uses param -> which uses pydot import. I tried and we cannot run the notebook without it

anupambhatnagar

In the notebook,

"Collecting PyTorch trace is covered in the official PyTorch recipe here. " does not contain the hyperlink.
Code Cell 3 can be cleaned up. there are two trace_dir statements.

anupambhatnagar · 2023-07-24T23:48:34Z

@briancoutinho Do you want to add documentation for this new feature?

anupambhatnagar · 2023-07-26T00:19:53Z

hta/common/execution_trace.py

+import sys
+import time
+
+from typing import List  # Dict, , NamedTuple, Optional


please remove commented code.

anupambhatnagar

Thank you for implementing execution trace support to HTA @briancoutinho

briancoutinho · 2023-07-26T18:31:28Z

@briancoutinho Do you want to add documentation for this new feature?

@anupambhatnagar i'll add it in a follow up :)

facebook-github-bot · 2023-07-26T18:31:40Z

@briancoutinho has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-07-26T18:58:59Z

@briancoutinho has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-07-26T22:24:00Z

@briancoutinho merged this pull request in c24333b.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 15, 2023

louisfeng self-requested a review July 18, 2023 17:33

briancoutinho marked this pull request as ready for review July 21, 2023 22:20

briancoutinho added 5 commits July 21, 2023 15:30

initial version reading execution trace and correlating it using rf_id

86e5519

update notebook

79d0072

fix third party and setup.py

2d8105f

add unit test, fix correlation to cpu_op/user annotation, improve doc…

5fa9d0a

… in example notebook

update circli and fix test conflict

5538797

briancoutinho force-pushed the read_execution_trace branch from b1479c2 to 5538797 Compare July 21, 2023 22:31

briancoutinho requested a review from anupambhatnagar July 21, 2023 22:32

minor cleanups

62459bd

anupambhatnagar reviewed Jul 24, 2023

View reviewed changes

anupambhatnagar reviewed Jul 26, 2023

View reviewed changes

follow up on comemnts

58c2822

anupambhatnagar self-requested a review July 26, 2023 18:09

anupambhatnagar approved these changes Jul 26, 2023

View reviewed changes

minor comment on requirements and update README

f05e26f

anupambhatnagar approved these changes Jul 26, 2023

View reviewed changes

facebook-github-bot closed this in c24333b Jul 26, 2023

facebook-github-bot added the Merged label Jul 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read and basic correlation for execution trace #57

Read and basic correlation for execution trace #57

briancoutinho commented Jul 15, 2023 •

edited

Loading

codecov-commenter commented Jul 21, 2023 •

edited

Loading

anupambhatnagar Jul 24, 2023

briancoutinho Jul 26, 2023

anupambhatnagar Jul 24, 2023

briancoutinho Jul 26, 2023

anupambhatnagar Jul 24, 2023

briancoutinho Jul 26, 2023

anupambhatnagar Jul 24, 2023

briancoutinho Jul 26, 2023

anupambhatnagar Jul 24, 2023

briancoutinho Jul 26, 2023

anupambhatnagar Jul 26, 2023

briancoutinho Jul 26, 2023

anupambhatnagar left a comment •

edited

Loading

anupambhatnagar commented Jul 24, 2023

anupambhatnagar Jul 26, 2023

anupambhatnagar left a comment

briancoutinho commented Jul 26, 2023

facebook-github-bot commented Jul 26, 2023

facebook-github-bot commented Jul 26, 2023

facebook-github-bot commented Jul 26, 2023

Read and basic correlation for execution trace #57

Read and basic correlation for execution trace #57

Conversation

briancoutinho commented Jul 15, 2023 • edited Loading

What does this PR do?

Before submitting

codecov-commenter commented Jul 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anupambhatnagar left a comment • edited Loading

Choose a reason for hiding this comment

anupambhatnagar commented Jul 24, 2023

Choose a reason for hiding this comment

anupambhatnagar left a comment

Choose a reason for hiding this comment

briancoutinho commented Jul 26, 2023

facebook-github-bot commented Jul 26, 2023

facebook-github-bot commented Jul 26, 2023

facebook-github-bot commented Jul 26, 2023

briancoutinho commented Jul 15, 2023 •

edited

Loading

codecov-commenter commented Jul 21, 2023 •

edited

Loading

anupambhatnagar left a comment •

edited

Loading